Adding a simple dataref extension #1018
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matzew
force-pushed
the
add_dataref_extension
branch
from
7c1fbd1 to
e84d5f0
Compare
duglin
reviewed
Feb 20, 2024
| const DataRefExtensionKey = "dataref" | ||
|
|
||
| type DataRefExtension struct { | ||
| DataRef string `json:"dataref"` |
There was a problem hiding this comment.
Not that it matters much but I'm curious why it's a struct instead of just a string? Are you leaving the option open that one day we may need to include extra metadata along with the URL?
|
yeah, to have it more flexible 🤷♂️
Sent from Gmail Mobile
…On Tue 20. Feb 2024 at 13:30, Doug Davis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In v2/extensions/dataref_extension.go
<#1018 (comment)>:
> +/*
+ Copyright 2024 The CloudEvents Authors
+ SPDX-License-Identifier: Apache-2.0
+*/
+
+package extensions
+
+import (
+ "github.com/cloudevents/sdk-go/v2/event"
+ "net/url"
+)
+
+const DataRefExtensionKey = "dataref"
+
+type DataRefExtension struct {
+ DataRef string `json:"dataref"`
Not that it matters much but I'm curious why it's a struct instead of just
a string? Are you leaving the option open that one day we may need to
include extra metadata along with the URL?
—
Reply to this email directly, view it on GitHub
<#1018 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABGPTUZAHNWNQI7YPPTCZDYUSJMZAVCNFSM6AAAAABDQULMHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJQGMYDSMZTGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
embano1
reviewed
Feb 20, 2024
Comment on lines
+14
to
+41
| func TestAddDataRefExtensionValidURL(t *testing.T) { | ||
| e := event.New() | ||
| expectedDataRef := "https://example.com/data" | ||
|
|
||
| err := AddDataRefExtension(&e, expectedDataRef) | ||
| if err != nil { | ||
| t.Fatalf("Failed to add DataRefExtension with valid URL: %s", err) | ||
| } | ||
| } | ||
|
|
||
| func TestAddDataRefExtensionInvalidURL(t *testing.T) { | ||
| e := event.New() | ||
| invalidDataRef := "://invalid-url" | ||
|
|
||
| err := AddDataRefExtension(&e, invalidDataRef) | ||
| if err == nil { | ||
| t.Fatal("Expected error when adding DataRefExtension with invalid URL, but got none") | ||
| } | ||
| } | ||
|
|
||
| func TestGetDataRefExtensionNotFound(t *testing.T) { | ||
| e := event.New() | ||
|
|
||
| _, ok := GetDataRefExtension(e) | ||
| if ok { | ||
| t.Fatal("Expected not to find DataRefExtension, but did") | ||
| } | ||
| } |
There was a problem hiding this comment.
Looks like Java unit tests :)
nit: can we use table-driven tests so those tests are easier to modify/add later? Example here:
There was a problem hiding this comment.
Just noticed you might be following the distributed tracing implementation which uses same style...whatever you prefer
There was a problem hiding this comment.
@matzew I'd like to merge this PR - did you want to update something based on this comment?
|
rebase needed |
duglin
force-pushed
the
add_dataref_extension
branch
from
e84d5f0 to
40188ae
Compare
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
duglin
force-pushed
the
add_dataref_extension
branch
from
40188ae to
58f801f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inspired by the java sdk, adding a go implementation for
datarefextension